Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GSP-837: Support Feature Flag #837

Merged
merged 15 commits into from
Oct 26, 2021
Merged

GSP-837: Support Feature Flag #837

merged 15 commits into from
Oct 26, 2021

Conversation

JinnyYi
Copy link
Contributor

@JinnyYi JinnyYi commented Oct 12, 2021

ref: #835

@JinnyYi JinnyYi changed the title Proposal: Support Feature Flag GSP-837: Support Feature Flag Oct 12, 2021
@JinnyYi JinnyYi requested a review from Xuanwo October 13, 2021 09:59
@Xuanwo
Copy link
Contributor

Xuanwo commented Oct 15, 2021

In GSP-87: Feature Gates, we have the concept of features first. And then in GSP-109: Redesign Features, we have a huge redesign about it.

We have the following features for now:

  • LoosePair
  • VirtualDir
  • VirtualObjectMetadata

It's obvious that they are only a subset of all features we support.

In fact, we can categorize all features into three kinds:

  • Native support
  • No native support but could be virtual
  • Can't support

In the past, we only care about features that "No native support but could be virtual". Because other features can be handled easily: if we support, everything will work fine; If not, we just don't implement the interface.

However, GSP-751: Write Empty File Behavior rise a new question: How to handle the different behavior of the same interface?

Maybe it's time for us to drop the idea that we introduced in GSP-1: Unify storager behavior.

Maybe we can merge all interface together into Storager and than implement:

type Storage interface {    
	Feature() Feature
}

if store.Feature().CanCopy() {
    err := x.Copy(oldpath, newpath)
    if err != nil {
        return err
    }
}

docs/rfcs/837-support-feature-flag.md Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
@JinnyYi JinnyYi requested a review from Xuanwo October 22, 2021 06:47
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
docs/rfcs/837-support-feature-flag.md Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Contributor

Xuanwo commented Oct 26, 2021

Mostly LGTM, let's create a tracking issue for this GSP and call for the final review.

@Xuanwo Xuanwo requested review from a team October 26, 2021 08:14
@Xuanwo Xuanwo enabled auto-merge (squash) October 26, 2021 09:12
@Xuanwo Xuanwo merged commit e541680 into master Oct 26, 2021
@Xuanwo Xuanwo deleted the issue-835 branch October 26, 2021 09:13
@Xuanwo
Copy link
Contributor

Xuanwo commented Oct 26, 2021

Bravo work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants